-
Notifications
You must be signed in to change notification settings - Fork 277
refactor: move use_reasoning to the model level from the category level to support non-reasoning models #178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…el to support non-reasoning models Signed-off-by: Huamin Chen <[email protected]>
✅ Deploy Preview for vllm-semantic-router ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@yuluo-yx can you review this? thanks |
👥 vLLM Semantic Team NotificationThe following members have been identified for the changed files in this PR and have been automatically assigned: 📁
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| return config, nil | ||
| } | ||
|
|
||
| // BoolPtr returns a pointer to a bool value (helper for tests and config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that this function was only used in the tests. Should it be moved into the tests section instead? The functional code doesn't use it. However, I don't think it's a major issue. It can be improved later.
| for _, category := range c.Categories { | ||
| if category.Name == categoryName { | ||
| if len(category.ModelScores) > 0 { | ||
| useReasoning := category.ModelScores[0].UseReasoning != nil && *category.ModelScores[0].UseReasoning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why just read the index0?
| reasoningStatus := "DISABLED" | ||
| if category.UseReasoning { | ||
| reasoningStatus = "ENABLED" | ||
| // Get the best model for this category (first in the list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how to define a best model in category? if it is based on score, maybe we should sort the list and get the biggest one? instead of reading the first index
| type ModelScore struct { | ||
| Model string `yaml:"model"` | ||
| Score float64 `yaml:"score"` | ||
| Model string `yaml:"model"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use_reasoning: true|false
reasoning_description: "Why reasoning is needed"
reasoning_effort: "low|medium|high"
should we move all these three reasoning related vars into model score?
|
@Xunzhuo thank you for the review. Would it be ok if we have this first and you help refactor the API to be more consistent next? Thanks. |
|
Sure @rootfs |
|
@Xunzhuo great! |
…el to support non-reasoning models (vllm-project#178) Signed-off-by: Huamin Chen <[email protected]>

What type of PR is this?
This allows fine grained reasoning control for each model
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Release Notes: Yes/No